fix(scripts): honor SPECKIT_PYTHON override for preset manifest parsing by chelsealong · Pull Request #4445 · github/spec-kit

chelsealong · 2026-09-04T13:41:39Z

Summary

Preset composition raises PyYAML is required to resolve preset template composition whenever the resolve/setup scripts are invoked under a bare
python3 that lacks PyYAML, while a uv tool install / pipx-isolated
CLI venv (which does have PyYAML, per pyproject.toml) is invisible to
that bare interpreter. This happens both when the bash/PowerShell twins
shell out to Python to parse a preset.yml manifest, and when the Python
twin (resolve_template.py / setup_plan.py / etc., via common.py)
tries to import yaml directly in its own process.

SPECKIT_PYTHON is already the established override for exactly this class
of problem elsewhere in the codebase (see
extensions/agent-context/scripts/bash/update-agent-context.sh and its
tests). This PR extends that same override to the three
template-resolution twins:

scripts/bash/common.sh: _python3_command() now prefers SPECKIT_PYTHON
(verified to be a working Python 3 interpreter that also has PyYAML)
before falling back to python3 / python / py -3 on PATH.
scripts/powershell/common.ps1: Get-Python3Command does the same.
scripts/python/common.py: since this twin runs in-process rather than
shelling out, _preset_template_layer now tries import yaml first and,
if that fails, delegates manifest parsing to SPECKIT_PYTHON via a small
subprocess-based safe_load/YAMLError shim (_import_yaml /
_DelegatedYAML) so the rest of the function's logic is unchanged.

No dependencies, lockfiles, or CI config were touched — pyyaml was already
a declared dependency; this only fixes which interpreter picks it up.

Update: an earlier revision of this PR let the bash/PowerShell twins
adopt SPECKIT_PYTHON as soon as it resolved to any Python 3 interpreter,
without checking it actually had PyYAML — unlike the precedent this PR
cites, which validates both. That meant a SPECKIT_PYTHON set to a valid
but PyYAML-less interpreter (e.g. pinned for an unrelated reason) would
shadow a working python3 on PATH and turn previously-succeeding preset
composition into a PyYAML is required failure. Both _python3_command()
and Get-Python3Command now also probe import yaml before accepting
SPECKIT_PYTHON, matching update-agent-context.sh's candidate-validation
loop; if the probe fails they fall through to the existing python3/
python/py -3 chain exactly as if SPECKIT_PYTHON were unset.

Test plan

Added test_all_variants_honor_speckit_python_override_when_yaml_missing to
tests/test_resolve_template_python_parity.py. It creates a real
PyYAML-less interpreter (python -m venv --without-pip) and:

runs the bash and Python twins with that interpreter as the default
python3 and no SPECKIT_PYTHON set — asserts both fail (reproduces
[Bug] Preset composition fails under isolated installs: scripts run on system python3 but PyYAML lives in the CLI venv #4443), then
sets SPECKIT_PYTHON to the interpreter that has PyYAML — asserts both
succeed and produce the correct composed template content.
(PowerShell is included in the same assertions when pwsh is available.)

Also added test_all_variants_fall_back_when_speckit_python_lacks_pyyaml,
covering the regression an earlier revision introduced: it sets
SPECKIT_PYTHON to a real Python-3 interpreter without PyYAML while
leaving PATH's own python3 (which has PyYAML) untouched, and asserts
composition still succeeds via that PATH fallback across the bash and
PowerShell twins.

Confirmed both tests fail without their respective fixes:

test_all_variants_honor_speckit_python_override_when_yaml_missing: with
git checkout HEAD~2 -- scripts/bash/common.sh scripts/powershell/common.ps1 scripts/python/common.py, fails with
assert False on the override-succeeds assertion.
test_all_variants_fall_back_when_speckit_python_lacks_pyyaml: with only
the latest commit's two files reverted (git checkout HEAD~1 -- scripts/bash/common.sh scripts/powershell/common.ps1, i.e. the state the
independent reviewer blocked), fails the same way — reproducing the
reviewer's exact regression scenario.
Restored the fix in both cases and reran; both pass.

$ .venv/bin/python -m pytest tests/test_resolve_template_python_parity.py -q
============================= 42 passed in 39.14s ==============================

$ .venv/bin/python -m pytest tests/extensions/test_extension_agent_context.py -q
======================== 39 passed, 1 skipped in 10.62s ========================

$ .venv/bin/python -m pytest tests/ -q
========== 7706 passed, 12 skipped, 48 warnings in 416.00s (0:06:56) ===========

$ shellcheck --severity=error scripts/bash/common.sh
(no output, exit 0)

$ uvx ruff@0.15.0 check tests/test_resolve_template_python_parity.py src
All checks passed!

AI disclosure

This PR was written by an autonomous coding agent (Claude/Anthropic) based on
the issue's description and existing SPECKIT_PYTHON precedent in the
codebase. An independent reviewer agent blocked the first revision for
letting the bash/PowerShell twins honor SPECKIT_PYTHON without checking
it actually had PyYAML (a regression relative to the cited precedent); this
revision fixes that specific gap, adds a regression test reproducing it,
and reruns the full test suite, shellcheck, and ruff before pushing.

Preset composition raises "PyYAML is required" whenever the resolve/setup scripts run under a bare `python3` that lacks PyYAML but a `uv tool install`/`pipx`-isolated CLI venv has it. SPECKIT_PYTHON is already the established override for this class of problem (see the agent-context extension's update-agent-context.sh); extend it to the bash, PowerShell, and Python template-resolution twins so a user (or the CLI, in future) can point scripts at an interpreter that actually has PyYAML. Fixes github#4443

…owerShell _python3_command() (bash) and Get-Python3Command (PowerShell) accepted SPECKIT_PYTHON as soon as it resolved to any Python 3 interpreter, unlike their cited precedent (update-agent-context.sh) which also verifies `import yaml` succeeds before trusting it. That let a SPECKIT_PYTHON without PyYAML shadow a PATH python3 that has it, turning previously working preset composition into a "PyYAML is required" failure. Gate the SPECKIT_PYTHON branch on a successful `import yaml` in both twins so it's only preferred when it can actually serve manifest parsing; otherwise the existing python3/python/py fallback chain runs unchanged, matching pre-fix behavior for that case.

Copilot

🟡 Changes recommended

Multiple moderate portability and manifest-parsing issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Extends SPECKIT_PYTHON support to preset manifest parsing across all script variants.

Changes:

Selects PyYAML-capable override interpreters in Bash and PowerShell.
Delegates YAML parsing from Python when needed.
Adds override and fallback parity tests.

File summaries

File	Review
`tests/test_resolve_template_python_parity.py`	Moderate: both tests use POSIX-only venv paths; baseline also fails to unset `SPECKIT_PYTHON`.
`scripts/python/common.py`	Moderate: JSON serialization rejects valid YAML values such as timestamps.
`scripts/powershell/common.ps1`	Override selection reviewed without additional findings.
`scripts/bash/common.sh`	Moderate: caller argument splitting breaks interpreter paths containing spaces.

Review details

Files reviewed: 4/4 changed files
Comments generated: 5
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- bash: preserve SPECKIT_PYTHON as one argv element instead of splitting it on whitespace, which broke paths containing spaces. - python: stringify non-JSON-native YAML values (e.g. unquoted dates) in the delegated-YAML subprocess instead of crashing json.dump. - tests: select venv paths by os.name so the SPECKIT_PYTHON parity tests work on Windows, and explicitly unset SPECKIT_PYTHON from the baseline env so a pre-set override can't mask the baseline failure.

chelsealong · 2026-09-08T16:22:29Z

Addressed all 5 findings from the Copilot review:

scripts/bash/common.sh: _python3_command now emits one argv element per line (mapfile -t), so a SPECKIT_PYTHON path containing spaces is no longer split across read -r -a.
scripts/python/common.py: the delegated-YAML subprocess now dumps with default=str, so non-JSON-native YAML values (e.g. an unquoted date) no longer crash the parse.
tests/test_resolve_template_python_parity.py: the two SPECKIT_PYTHON tests now pick venv paths via a new venv_python3_exe() helper (POSIX bin/python3 vs Windows Scripts/python.exe), and explicitly unset SPECKIT_PYTHON before building the baseline env. Added two new regression tests covering the spaces-in-path and non-JSON-YAML-value fixes.

Verified both new tests fail on the pre-fix code (git checkout HEAD~1 -- <file>) and pass after; full suite (pytest tests/ -q) is green at 7708 passed.

Copilot

🟡 Changes recommended

Bash 3.2 incompatibility and delegated YAML transport/validation regressions remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/python/common.py:412

text=True uses the process locale for the pipe. Under the repository's tested ASCII-locale configuration, a valid manifest containing non-ASCII metadata raises UnicodeEncodeError before the delegated interpreter can parse it. Force UTF-8 for both the parent pipe and the child Python stdio.

scripts/bash/common.sh:520

This second mapfile call also breaks under the supported macOS system Bash 3.2, so preset lookup cannot use the selected interpreter there. Use the Bash-3-compatible while read array population while retaining each emitted command argument as a single element.

        mapfile -t python_cmd < <(_python3_command)

scripts/bash/common.sh:640

mapfile requires Bash 4+, but sh is the default on macOS where /bin/bash is 3.2. Consequently composed preset resolution still fails on the environment reported by #4443 even when SPECKIT_PYTHON is valid; use the portable while read form here as well.

        mapfile -t python_cmd < <(_python3_command)

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

mnriem · 2026-09-09T17:14:20Z

The re-review and CI both flag a real regression: your mapfile -t change in _python3_command (from the argv-splitting fix) uses a Bash 4.0+ builtin, but macOS ships Bash 3.2 — CI shows common.sh: line 640: mapfile: command not found on macOS, which fails preset composition (test_all_variants_resolve_requested_template[composed]). Please replace mapfile with a 3.2-safe read loop (e.g. while IFS= read -r line; do arr+=("$line"); done < <(...)), and address the delegated-YAML transport/validation point the review raised. Re-request once CI is green across macOS/Windows.

…types across delegation mapfile is a Bash 4+ builtin unavailable on macOS's system Bash 3.2, so preset resolution failed there even with a valid SPECKIT_PYTHON. The delegated-YAML subprocess also used the process locale for its pipe (raising under an ASCII locale on non-ASCII metadata) and stringified every non-JSON-native YAML value indiscriminately, letting a validated field like `file` silently pass an isinstance(str) check that native parsing would correctly reject.

chelsealong · 2026-09-10T00:21:24Z

Addressed both points from the latest review:

scripts/bash/common.sh: replaced all three mapfile -t python_cmd < <(_python3_command) calls with a Bash 3.2-compatible while IFS= read -r ...; do ...; done loop (macOS system Bash is 3.2 and lacks mapfile).
scripts/python/common.py: _DelegatedYAML.safe_load now passes encoding="utf-8" (instead of text=True) and sets PYTHONIOENCODING=utf-8 for the child process, so non-ASCII manifest metadata survives an ASCII-locale environment. It also replaced the blanket default=str JSON dump with a marker-based default/object_hook pair, so a validated field (e.g. file) that PyYAML parses into a non-JSON-native type (an unquoted date) still fails the isinstance(str) check under delegation, matching native yaml.safe_load behavior instead of silently passing as a string.

Added three regression tests to tests/test_resolve_template_python_parity.py, each confirmed to fail on the pre-fix code and pass after: test_bash_resolves_composed_template_without_bash4_mapfile_builtin (simulates Bash 3.2 by disabling the mapfile builtin), test_python_variant_delegates_manifest_with_non_ascii_metadata_under_ascii_locale, and test_python_variant_rejects_delegated_manifest_with_non_string_validated_field. Full suite: pytest tests/ -q → 7711 passed, 12 skipped. shellcheck --severity=error scripts/bash/*.sh and ruff@0.15.0 check src tests are clean.

Copilot

🟡 Changes recommended

Delegated parsing rejects some valid YAML structures, leaks child tracebacks, and one regression test does not prove override selection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tests/test_resolve_template_python_parity.py:766

This assertion can pass even if the spaced override is ignored, because PATH still contains a working Python with PyYAML and the resolver silently falls back to it. Also, a venv created with --system-site-packages from the test venv does not necessarily inherit packages installed in that parent venv. Assert that the spaced interpreter imports PyYAML and that _python3_command actually selects it (or make every PATH fallback PyYAML-less) so the regression test fails when whitespace splitting returns.

scripts/python/common.py:433

The delegated transport cannot serialize every value that yaml.safe_load accepts. In particular, json.dump(..., default=...) never applies default to mapping keys, so an otherwise ignored metadata mapping such as metadata: {2026-09-08: value} raises TypeError here, while the in-process parser accepts the same valid YAML and resolves the template. Preserve non-JSON-native mapping keys with a tagged recursive encoding (or move manifest extraction/validation into the child) so delegation does not change which manifests resolve.

                    "json.dump(yaml.safe_load(sys.stdin.read()), sys.stdout, default=_default)",

Files reviewed: 5/5 changed files
Comments generated: 1
Review effort level: Balanced

…backs json.dump's default= hook never applies to dict keys, so a manifest mapping with a non-JSON-native key (e.g. an unquoted date) raised TypeError under SPECKIT_PYTHON delegation even though the in-process parser accepts it; stringify non-native keys before dumping. Also catch yaml.YAMLError in the delegated child so a malformed manifest reports a concise message instead of leaking the child's raw traceback into TemplateResolutionError. Fixes the spaced-path regression test relying on --system-site-packages inheritance and not proving the override was actually selected.

chelsealong · 2026-09-10T15:26:48Z

Addressed both points from the latest Copilot review:

scripts/python/common.py: the delegated-YAML child now stringifies non-JSON-native mapping keys before json.dump (its default= hook only applies to values, not keys, so a manifest like metadata: {2026-09-08: value} raised TypeError under delegation even though the in-process parser accepts it). The child also catches yaml.YAMLError and prints a concise message instead of letting a raw Python traceback leak into TemplateResolutionError on a malformed manifest.
tests/test_resolve_template_python_parity.py: reworked test_bash_honors_speckit_python_path_containing_spaces to symlink sys.executable into the spaced path (guaranteed PyYAML, unlike relying on --system-site-packages inheritance) and put a PyYAML-less venv on PATH, so the test can only pass if _python3_command actually selects the spaced override. Added regression tests for the mapping-key and traceback-leak fixes.

Confirmed both new regression tests fail on the pre-fix code and pass after. Full suite: pytest tests/ -q → 7713 passed, 12 skipped. shellcheck --severity=error scripts/bash/*.sh and ruff@0.15.0 check tests/test_resolve_template_python_parity.py src are clean.

mnriem · 2026-09-10T18:30:24Z

Thanks @chelsealong — confirmed both are handled: the child now emits the concise yaml.YAMLError message instead of a raw traceback, and _stringify_keys correctly covers non-JSON-native mapping keys that default= misses. This is on me next: I'll trigger CI on the new head and review.

Copilot

🟡 Changes recommended

Delegated parsing can reject valid recursive YAML, and interpreter and test portability guards remain incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/python/common.py:486

The Python twin accepts any executable that can import yaml, unlike the Bash/PowerShell twins and the established override contract. If SPECKIT_PYTHON resolves to a Python 2 environment with PyYAML, the probe succeeds and the later child program fails with syntax/runtime errors instead of treating the override as unusable. Include the major-version check in this probe.

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

+                    "def _stringify_keys(obj):\n"
+                    "    if isinstance(obj, dict):\n"
+                    "        return {\n"
+                    "            (k if isinstance(k, (str, int, float, bool)) or k is None else str(k)): _stringify_keys(v)\n"
+                    "            for k, v in obj.items()\n"
+                    "        }\n"
+                    "    if isinstance(obj, list):\n"
+                    "        return [_stringify_keys(v) for v in obj]\n"


+    spaced_dir = tmp_path / "tool env"
+    spaced_dir.mkdir()
+    spaced_exe = spaced_dir / Path(sys.executable).name
+    spaced_exe.symlink_to(sys.executable)


… 3 in override probe - scripts/python/common.py: _stringify_keys now tracks visited container ids so a self-referential YAML alias (e.g. metadata: &m {self: *m}, which yaml.safe_load supports natively) no longer causes RecursionError during SPECKIT_PYTHON delegation. _import_yaml's probe now also checks sys.version_info.major == 3, matching the bash/PowerShell twins, so a SPECKIT_PYTHON pointing at a PyYAML-equipped Python 2 is rejected instead of being accepted and failing later in the child process. - tests/test_resolve_template_python_parity.py: fixed test_bash_honors_speckit_python_path_containing_spaces, which was failing in CI (PyYAML is required to resolve preset template composition) because symlinking sys.executable from outside a venv's own directory breaks Python's pyvenv.cfg discovery, hiding the venv's site-packages. Replaced the symlink with a shim script that execs sys.executable directly, which keeps discovery working. Added regression tests for the two fixes above.

chelsealong · 2026-09-11T10:13:05Z

Addressed both points from the latest Copilot review, plus the failing CI:

scripts/python/common.py: _stringify_keys now tracks visited container ids, so a self-referential YAML alias (metadata: &m {self: *m}, which yaml.safe_load accepts natively) no longer causes a RecursionError under delegation. _import_yaml's probe now also checks sys.version_info.major == 3 before accepting SPECKIT_PYTHON, matching the bash/PowerShell twins, so a PyYAML-equipped Python 2 is rejected instead of failing later in the child process.
Also fixed the CI failure on test_bash_honors_speckit_python_path_containing_spaces (all 6 matrix jobs): reproduced locally under uv run pytest — symlinking sys.executable from outside its own venv directory breaks Python's pyvenv.cfg discovery, so the symlinked interpreter silently loses access to the venv's PyYAML. Replaced the symlink with a shim script that execs sys.executable directly (keeping argv[0] at its real, discoverable path), which also drops the need for os.symlink (addressing the earlier Windows-privilege concern on that same test).

Added test_python_variant_delegates_manifest_with_recursive_yaml_alias and test_python_variant_rejects_speckit_python_override_without_python_3, each confirmed to fail on the pre-fix code and pass after. Full suite: uv run pytest tests/ → 7715 passed, 12 skipped. ruff@0.15.0 check is clean.

chelsealong added 2 commits September 4, 2026 13:17

chelsealong requested a review from mnriem as a code owner September 4, 2026 13:41

mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 8, 2026

mnriem requested a balanced review from Copilot September 8, 2026 15:19

Copilot started reviewing on behalf of mnriem September 8, 2026 15:19 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread scripts/bash/common.sh

Comment thread scripts/python/common.py Outdated

Comment thread tests/test_resolve_template_python_parity.py Outdated

Comment thread tests/test_resolve_template_python_parity.py

Comment thread tests/test_resolve_template_python_parity.py Outdated

mnriem requested a balanced review from Copilot September 9, 2026 14:10

Copilot started reviewing on behalf of mnriem September 9, 2026 14:10 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread scripts/bash/common.sh Outdated

Comment thread scripts/python/common.py Outdated

mnriem added the author-awaiting Waiting on author response label Sep 9, 2026

mnriem requested a balanced review from Copilot September 10, 2026 13:57

Copilot started reviewing on behalf of mnriem September 10, 2026 13:58 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread scripts/python/common.py Outdated

mnriem removed the author-awaiting Waiting on author response label Sep 10, 2026

mnriem requested a balanced review from Copilot September 10, 2026 18:30

Copilot started reviewing on behalf of mnriem September 10, 2026 18:31 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Uh oh!

Conversation

chelsealong commented Sep 4, 2026

Summary

Test plan

AI disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chelsealong commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

mnriem commented Sep 9, 2026

Uh oh!

chelsealong commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

chelsealong commented Sep 10, 2026

Uh oh!

mnriem commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

chelsealong commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mnriem commented Sep 10, 2026 •

edited

Loading